Adds support for external imagery providers in CesiumIonOverlay #1385
Conversation
Extends CesiumIonOverlay to support external imagery providers like Google Maps and Bing Maps, fetching metadata and adjusting tile URL structures accordingly. Updates the example to showcase this new feature by adding base map options for different map providers.
gkjohnson
left a comment
gkjohnson
left a comment
There was a problem hiding this comment.
This is great! Thanks for the work - I've made some comments.
|
|
||
| return this.subdomains[ Math.floor( Math.random() * this.subdomains.length ) ]; | ||
|
|
There was a problem hiding this comment.
Can you explain this? What's the purpose? You're randoming fetching from a set of urls every time?
There was a problem hiding this comment.
It's explained here: https://learn.microsoft.com/en-us/bingmaps/rest-services/directly-accessing-the-bing-maps-tiles
The
imageUrlSubdomainsproperty in the imagery metadata response provides a list of valid subdomain that can be used in the tile URL. Using a different one for each tile request you can increase performance by get around browser URL request limits i.e. many browsers allow up to 8 concurrent requests to the same domain. Using subdomains allows up to 8 requests per subdomain.
Those browser request limits shouldn't actually be a problem anymore with HTTP/2.
There was a problem hiding this comment.
Oh interesting thanks for the reference. This is very bing-API specific so let's add a comment with a link to that explanation.
There was a problem hiding this comment.
Actually, some OSM providers also have multiple subdomains, with similar statement:
{s}should be replaced by a subdomain. The exact values are in the tile server's documentation, but typically it is a single lettera, b, c[...] Subdomains are used to help with browser parallel requests per domain limitation (not needed if the webserver uses http/2), so consider switching letters at runtime
There was a problem hiding this comment.
Interesting - this may be a more basic feature to include at the "TiledImageSource" level so subdomains can be used if their available. Something for another day if it's of interest.
|
|
||
| } | ||
|
|
||
| // this.imageSource.url = json.url; |
There was a problem hiding this comment.
nit: lets remove this comment if it's not needed
| } ) ); | ||
|
|
||
| this.imageSource.url = json.url; | ||
| if ( json.type !== 'IMAGERY' ) throw new Error( 'Only IMAGERY is supported as overlay type' ); |
There was a problem hiding this comment.
nit: Lets prepend this with with the name of the plugin so users know where the error was thrown from:
throw new Error( 'CesiumIonOverlay: Only IMAGERY is supported as overlay type' );| `${json.options.url}/REST/v1/Imagery/Metadata/${json.options.mapStyle}?incl=ImageryProviders&key=${json.options.key}&uriScheme=https`, | ||
| ).then( ( res ) => res.json() ); | ||
| const metadata = response.resourceSets?.[ 0 ]?.resources?.[ 0 ]; | ||
| if ( ! metadata ) throw new Error( 'Could not retrieve Bing Maps metadata' ); |
There was a problem hiding this comment.
Same with this error.
Also, is there a known case where this happens? If it's just defensive then I'd prefer to remove the check.
There was a problem hiding this comment.
Yes, it's just defensive. The resources should always be set. Or in other words: I didn't encounter anything else.
There was a problem hiding this comment.
Okay lets remove it in that case.
| this.imageSource.fetchData = ( ...args ) => this.fetch( ...args ); | ||
| this._attributions = []; | ||
|
|
||
| this.externalType = null; |
There was a problem hiding this comment.
For clarity lets make this a boolean.
| this.imageSource = new XYZImageSource( { | ||
| ...this.options, | ||
| url: `${json.options.url}/v1/2dtiles/{z}/{x}/{y}?session=${json.options.session}&key=${json.options.key}`, | ||
| tileDimension: json.options.tileWidth, | ||
| } ); |
There was a problem hiding this comment.
Is there a "known" number of tile levels for Google Map Tiles we can set here?
There was a problem hiding this comment.
This information is not specified within options. But Google says it goes up to 22 (https://developers.google.com/maps/documentation/tile/2d-tiles-overview?hl=en). So, it would be hard-coded here.
There was a problem hiding this comment.
Great, lets hard code it with a link to that page.
| ...this.options, | ||
| url: json.url, | ||
| } ); | ||
| this.imageSource.fetchData = ( ...args ) => this.fetch( ...args ); |
There was a problem hiding this comment.
It shouldn't be necessary to assign "fetchData" here since it's assigned above.
There was a problem hiding this comment.
But I have removed it in the constructor. All imageSource initialization is now here. Or did I miss something?
There was a problem hiding this comment.
I see now - it's initialized in all other constructors but replaced in the "ImageOverlayPlugin" section. In that case we should be assigning imageSource.fetchData for all image source creation cases, which we should just be able to do after the switch statement.
2 participants
Extends CesiumIonOverlay to support external imagery providers like Google Maps and Bing Maps, fetching metadata and adjusting tile URL structures accordingly.
Updates the example to showcase this new feature by adding base map options for different map providers.
Closes #1382